Looks like 545cce2 (#11603) had knock-on implications for other tests. Will address.

I've already tried twice to propose PR to integrate pre-commits and they were all refused because involving "too much changes" my last one (that you linked) is creating the file itself but only runs ruff. Can someone provides review there ?

That's already the second time that I try to contribute to sphinx and that my PR is closed because overridden by someone else. Is it common practice here ? if yes I'll stop trying and will simply open well documented issues.

I've already tried twice to propose PR to integrate pre-commits and they were all refused because involving "too much changes" my last one (that you linked) is creating the file itself but only runs ruff. Can someone provides review there ?

That's already the second time that I try to contribute to sphinx and that my PR is closed because overridden by someone else. Is it common practice here ? if yes I'll stop trying and will simply open well documented issues.

It's not common practice. However, you've overloaded the PRs in all cases. Rather than just focusing on simply integrating a new tool, the PRs have also reformatted the entire code base with a different tool (#11251) or changed the rules for an existing linter (#11375). I suspect you'd have more luck if you tackled these topics in separate PRs.

@AA-Turner I don't want to merge this without getting your agreement on this, given the imbalance in number of contributions of late. As such, could I get your ack on this? fwict, you're using a local virtual environment to (presumably) enable IDE integration? If so, this should have no significant impact for you. I suspect the only real change would be whenever we want to bump a linter version we would now update the pacakge version in .pre-commit-config.yaml rather than in .github/workflows/lint.yaml (so a different YAML file). From my end, this would make my own workflow significantly easier since it aligns with how things work in virtually ever project I contribute to (I work in vim and rely on pre-commit to catch lint failures as I commit).

fwict, you're using a local virtual environment to (presumably) enable IDE integration?

I use a single conda environment (through the mamba reimplementation) for development of Sphinx that contains dependencies and associated tools (e.g. sphinx-lint, ruff). I then run these tools from the command line, though I use PyCharm as an editor. My main objection continues to be that I don't understand the point/benefits of pre-commit -- every tool is faster when run on its own, and we don't have a suite of thousands of linting tools. Personally, I run ruff every so often, and python -m sphinx build -M html doc build -anETW --keep-going if I've changed the documentation. isort and flake8 could probably be removed at this point, but I'm not 100% sure of the Ruff implementation so they're kept for surety.

this should have no significant impact for you

If I make no changes, yes, though I feel that contributors ought adopt the guidelines set out by the project, so I'd feel obliged to change to using pre-commit, which would slow down my workflow -- I tend to make very frequent small commits that I later extensively rebase into a final PR. If each of these had a 5-15 second delay it would be rather annoying.

The only reason I don't use tox is that I use Windows & I have had various problems with it -- I'd prefer to remove it entirely, but I recognise that it is used by various people.

A

fwict, you're using a local virtual environment to (presumably) enable IDE integration?

I use a single conda environment (through the mamba reimplementation) for development of Sphinx that contains dependencies and associated tools (e.g. sphinx-lint, ruff). I then run these tools from the command line, though I use PyCharm as an editor. My main objection continues to be that I don't understand the point/benefits of pre-commit -- every tool is faster when run on its own, and we don't have a suite of thousands of linting tools. Personally, I run ruff every so often, and python -m sphinx build -M html doc build -anETW --keep-going if I've changed the documentation. isort and flake8 could probably be removed at this point, but I'm not 100% sure of the Ruff implementation so they're kept for surety.

For me at least, the main benefit is that linting becomes an automated thing rather than something run as an afterthought (or not at all). You can opt to ignore the linting step (pass -n to git commit) but tbh, I don't see why I'd want to be committing syntactically incorrect code even if I am rapidly hacking on something.

this should have no significant impact for you

If I make no changes, yes, though I feel that contributors ought adopt the guidelines set out by the project, so I'd feel obliged to change to using pre-commit, ...

Do we? I mean, does it matter if you use conda and I use pre-commit (and someone else uses tox) so long as the CI is happy? Heck, even the tools we use aren't even important: we only care about the output. If someone were to only use ruff (or no linters!) but their code passed our checks in the gate, more power to them, right?

Now if we were proposing a process that no one ran, that's different, but this is slotted into our CI so we are dogfooding this process.

...which would slow down my workflow -- I tend to make very frequent small commits that I later extensively rebase into a final PR. If each of these had a 5-15 second delay it would be rather annoying.

5-15 seconds seems exceptionally long. fwiw pre-commit only runs checks against the files you modified in a given commit and in my experience this often takes roughly a second (things like commits that refactor dozens of files aside, but these are very much the exception and you always have the git commit -n escape hatch).

The only reason I don't use tox is that I use Windows & I have had various problems with it -- I'd prefer to remove it entirely, but I recognise that it is used by various people.

gtk. From a Linux user's perspective it is obviously very useful, so thank you for keeping it around.

Although I thinking linting and enforcement of code style using continuous integration, I often feel a bit conflicted about pre-commit. Linting sets a policy for code style but doesn't restrict the tools that developers can use, or have to use, to meet that policy. pre-commit goes slightly further and channels developers through a certain set of workflow tools. From a diversity and choice perspective that makes me question it.

(I tend to use vim only, and manually run lint-checkers and tests from the commandline where possible. I don't doubt that it makes me less productive under many circumstances, and perhaps the resulting corrective commit noise is annoying sometimes, but it reassures me that I'm contributing work that I've developed -- and hopefully understand and remember -- by myself)

To try to make that feedback more relevant here: I'd personally prefer if this were opt-in by default rather than opt-out.

To try to make that feedback more relevant here: I'd personally prefer if this were opt-in by default rather than opt-out.

It is opt-in, no? You need to install the tool itself and configure it in your local repo before anything will happen. If you opt to install the linters locally instead, that's a-okay.

Linting sets a policy for code style but doesn't restrict the tools that developers can use

At the end of the day for a PR to be validated, you need to comply with the ruff checks, so what tool choices do you have ? pre-commit only brings to your local development the linting operation that are/should be tested in the CI.

I'm probably being too stubborn - it was the mention of "(pass -n to git commit)" that made me think that this would require opt-out. I'm not hugely familiar with git hooks, but I seem to remember that they can be activated-by-default in some repos.

As for alternatives: there are many linters, but there's only one pre-commit -- it seems to be a widely-used de-fact standard. I'm sort-of-ok with that, but personally it feels slightly more dynamic (in terms of downloading binaries from git repositories that run on local developer machines) than I'm comfortable with. I'm not aware of any problems with it, but it's the kind of project that I'd be terrified to maintain (due to the level of usage and effect it can have, and the potential levels of responsibility and pressure as a result), and that makes me cautious too.

And to be honest: sometimes I do just code a fix (or equally often, poor attempt at one) and then lean on continuous integration to catch problems. That's more likely in cases where I don't totally trust the install processes for the continuous integration components.

I'm probably being too stubborn - it was the mention of "(pass -n to git commit)" that made me think that this would require opt-out. I'm not hugely familiar with git hooks, but I seem to remember that they can be activated-by-default in some repos.

Ah yes, the pre-commit hook is a client-side hook. From the Git Book:

It’s important to note that client-side hooks are not copied when you clone a repository. If your intent with these scripts is to enforce a policy, you’ll probably want to do that on the server side; see the example in An Example Git-Enforced Policy.

So you can completely ignore this if you like. I'll update the PR text.

As for alternatives: there are many linters, but there's only one pre-commit -- it seems to be a widely-used de-fact standard. I'm sort-of-ok with that, but personally it feels slightly more dynamic (in terms of downloading binaries from git repositories that run on local developer machines) than I'm comfortable with. I'm not aware of any problems with it, but it's the kind of project that I'd be terrified to maintain (due to the level of usage and effect it can have, and the potential levels of responsibility and pressure as a result), and that makes me cautious too.

Indeed. It's important to note, however, that pre-commit is just a runner of existing linters and of course tox, make or even bash could fulfil the same role. At present, we don't have any runner that is capable of running all our linters: instead, users need to create their own tooling/configure a suitable environment. This provides at least a mechanism for doing this.

And to be honest: sometimes I do just code a fix (or equally often, poor attempt at one) and then lean on continuous integration to catch problems. That's more likely in cases where I don't totally trust the install processes for the continuous integration components.

That's a-okay, but of course not everyone works like that however. This PR at least provides an option for the latter folks.

To be clear, I want to re-add the ability to install and run the correct version of all our linters with one command. As far as I'm aware, our three options here are pre-commit, tox, and nox. I have a preference for pre-commit since it's can be automatic (if installed) and is easily wrapped by tox as I've done here, but I will settle for any of the three. However, whatever tool we settle on should be used by CI to prevent it going stale.

I'm keeping an eye on this thread, as introducing a standardised command mechanism would meet the goal of running a single command and mean that using pre-commit is still optional. We may also get this 'for free' if we switch to uv, rye, or a similar tool for workflow management (as a different PR proposes). However for now closing this one -- if no progress is made then we can revisit this.

A